cli: support --file option in config commands - #10005
Conversation
|
cc @josephlou5 (as discussed on #9541) . As I don't have access yet to add reviewers. Note: Added myself to |
josephlou5
left a comment
There was a problem hiding this comment.
Could you also update docs/config.md with some text about this flag?
|
The "unattended automation" part seems dangerous, when other discussions were focused on the security of the config files. |
|
Does there also need to be any more handling for my comment #9541 (comment)? It would be nice if |
bed312f to
d36171e
Compare
Done. |
I agree that now as |
Just to clarify, the interactive prompt was only there to disambiguate between multiple config files, not as a security gate. Commands like jj config set --repo (and git config) were running non-interactively, and any process running jj could already edit the config files on disk directly anyway. I also added path validation so --file can only target valid jj config locations. I've updated the wording in the changelog and commit message to avoid confusion! |
josephlou5
left a comment
There was a problem hiding this comment.
Btw you can resolve comment threads once you've addressed them: https://docs.jj-vcs.dev/latest/contributing/#code-reviews
| } | ||
| } | ||
|
|
||
| // 5. Check system config paths (/etc/jj/config.toml or files in /etc/jj/conf.d) |
There was a problem hiding this comment.
Does this mean jj config edit --file /etc/jj/config.toml will work?
There was a problem hiding this comment.
Yes. As long as the user/process has write permissions for /etc/jj/ (e.g. running under sudo or as root), jj config {edit,set,unset} --file /etc/jj/config.toml will work and target it as ConfigSource::System.
If run without write permissions, it will fail with a standard permission denied error.
There was a problem hiding this comment.
AFAIK this is new behavior. Should it be documented?
There's also a question of whether it should be possible, as I think these system files should usually be provided rather than available to be edited. Obviously if you have permission you can do whatever, but Jujutsu thus far has simply said "system configuration is loaded from here" and that was the end of the story. I guess there isn't a huge reason to not do this, but just saying it felt a little surprising to me :)
If we keep this, I think it should be documented.
There was a problem hiding this comment.
Thanks for the context! I don't feel strongly either way. My thinking was just consistency: since --file allows targeting specific config files, supporting system config paths felt natural. I've updated the docs accordingly.
I am also okay to remove if we eventually consider that. Let me know what you think.
| /// See `jj config edit` if you'd like to immediately edit a file. | ||
| #[derive(clap::Args, Clone, Debug)] | ||
| #[group(id = "config_level", multiple = false, required = true)] | ||
| pub struct ConfigPathArgs { |
There was a problem hiding this comment.
Nit: Maybe add a regular // comment explaining why the common ConfigTargetArgs can't be reused here? (Because this command doesn't accept --file.)
Yes, that sounds good. You can keep it in this PR if you'd like (and I would recommend that too). |
| /// Target the workspace-level config | ||
| #[arg(long)] | ||
| workspace: bool, | ||
| } |
There was a problem hiding this comment.
Can you leave the original ConfigLevelArgs unmodified? I don't think jj config list should support --file=PATH either.
There was a problem hiding this comment.
Thanks for the feedback. Yeah, I agree. I guess I misunderstood how list worked on the first pass. I've reverted ConfigLevelArgs, and introduced ConfigTargetArgs specifically for the modifying commands (edit, set, unset). Could you please have another look on that?
a521699 to
8e8696f
Compare
8e8696f to
cc38b45
Compare
9476eb6 to
7f8386a
Compare
7f8386a to
32395a5
Compare
069d007 to
f5f836b
Compare
Allow targeting a specific config file path with `--file` in
`jj config {edit,set,unset}`.
This enables explicit targeting of specific configuration files (such
as files in `conf.d/` or custom files loaded via `--config-file`) and
avoids interactive prompts when multiple config files exist.
The `--file` option validates that the target path is a recognized `jj`
configuration location to prevent creating untracked or arbitrary files.
Fixes jj-vcs#9541
8b19193 to
1a93ab7
Compare
When modifying user configuration (`jj config {edit,set,unset} --user`),
always target the primary user configuration file (`~/.config/jj/config.toml`
or `~/.jjconfig.toml`) rather than prompting when drop-in files exist in
`conf.d/`.
Specific drop-in files in `conf.d/` can now be targeted explicitly with
the `--file` option.
Fixes jj-vcs#9541
1a93ab7 to
be0bdbc
Compare
| #[test] | ||
| fn test_config_set_file_with_existing_scopes() -> TestResult { | ||
| let mut test_env = TestEnvironment::default(); | ||
| let conf_d = test_env.config_path().parent().unwrap().join("conf.d"); |
There was a problem hiding this comment.
nit: test_env.env_root().join("conf.d") ?
| // Using JJ_CONFIG allows targeting any custom file with --file | ||
| let env_file = test_env.env_root().join("from_env.toml"); | ||
| std::fs::write(&env_file, "")?; | ||
| let output = work_dir.run_jj_with(|cmd| { | ||
| cmd.env("JJ_CONFIG", &env_file); | ||
| cmd.args([ | ||
| "config", | ||
| "set", | ||
| "--file", | ||
| env_file.to_str().unwrap(), | ||
| "env-key", | ||
| "env-val", | ||
| ]) | ||
| }); | ||
| insta::assert_snapshot!(output, @""); | ||
| assert_eq!( | ||
| std::fs::read_to_string(&env_file)?, | ||
| "env-key = \"env-val\"\n" | ||
| ); |
There was a problem hiding this comment.
nit: This seems redundant because JJ_CONFIG is set globally by the test runner.
| // Using global --config-file allows targeting any custom file with --file | ||
| let custom_file = test_env.env_root().join("outside.toml"); | ||
| std::fs::write(&custom_file, "")?; | ||
| let output = work_dir.run_jj([ | ||
| "--config-file", | ||
| custom_file.to_str().unwrap(), | ||
| "config", | ||
| "set", | ||
| "--file", | ||
| custom_file.to_str().unwrap(), | ||
| "outside-key", | ||
| "outside-val", | ||
| ]); | ||
| insta::assert_snapshot!(output, @""); | ||
| assert_eq!( | ||
| std::fs::read_to_string(&custom_file)?, | ||
| "outside-key = \"outside-val\"\n" | ||
| ); |
There was a problem hiding this comment.
nit: Do you think this behavior is useful? If not, can you update the comment to note that it can be removed if necessary?
| Hint: Valid config locations include user configs (`~/.config/jj/config.toml` or `conf.d/*.toml`), repo/workspace configs, or files loaded with the global flag `--config-file <PATH>`. | ||
| [EOF] | ||
| [exit status: 1] | ||
| "); |
There was a problem hiding this comment.
nit: Can you add a test for a valid relative path?
| fn test_config_set_user_with_conf_d() -> TestResult { | ||
| let mut test_env = TestEnvironment::default(); | ||
| let config_dir = test_env.config_path().parent().unwrap(); | ||
| let config_file = config_dir.join("config.toml"); | ||
| let conf_d = config_dir.join("conf.d"); | ||
| std::fs::create_dir_all(&conf_d)?; | ||
| std::fs::write(&config_file, "foo = 'from_config'\n")?; | ||
| std::fs::write(conf_d.join("work.toml"), "bar = 'from_work'\n")?; | ||
| let user_config_path = join_paths([&config_file, &conf_d])?; | ||
| test_env.set_config_path(&user_config_path); | ||
|
|
||
| // Setting with --user targets config.toml directly without prompting | ||
| let output = test_env.run_jj_in(".", ["config", "set", "--user", "foo", "updated_foo"]); | ||
| insta::assert_snapshot!(output, @""); | ||
| assert_eq!( | ||
| std::fs::read_to_string(&config_file)?, | ||
| "foo = \"updated_foo\"\n" | ||
| ); | ||
| assert_eq!( | ||
| std::fs::read_to_string(conf_d.join("work.toml"))?, | ||
| "bar = 'from_work'\n" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_config_set_user_targets_first_file_when_only_conf_d_exists() -> TestResult { | ||
| let mut test_env = TestEnvironment::default(); | ||
| let config_dir = test_env.config_path().parent().unwrap(); | ||
| let config_file = config_dir.join("config.toml"); | ||
| let conf_d = config_dir.join("conf.d"); | ||
| std::fs::create_dir_all(&conf_d)?; | ||
| std::fs::write(conf_d.join("01_work.toml"), "bar = 'from_work'\n")?; | ||
| std::fs::write(conf_d.join("02_home.toml"), "baz = 'from_home'\n")?; | ||
| let user_config_path = join_paths([&config_file, &conf_d])?; | ||
| test_env.set_config_path(&user_config_path); | ||
|
|
||
| // config.toml does not exist | ||
| assert!(!config_file.exists()); | ||
|
|
||
| // Setting with --user targets the first file in conf.d | ||
| let output = test_env.run_jj_in(".", ["config", "set", "--user", "foo", "new_foo"]); | ||
| insta::assert_snapshot!(output, @""); | ||
| assert!(!config_file.exists()); | ||
| assert_eq!( | ||
| std::fs::read_to_string(conf_d.join("01_work.toml"))?, | ||
| "bar = 'from_work'\nfoo = \"new_foo\"\n" | ||
| ); | ||
| assert_eq!( | ||
| std::fs::read_to_string(conf_d.join("02_home.toml"))?, | ||
| "baz = 'from_home'\n" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
These tests look redundant. We already have jj config set/edit with multiple files.
Allow targeting a specific config file path with
--fileinjj config {edit,set,unset}.This enables explicit targeting of specific configuration files (such
as files in
conf.d/or custom files loaded via--config-file) andavoids interactive prompts when multiple config files exist.
The
--fileoption validates that the target path is a recognizedjjconfiguration location to prevent creating untracked or arbitrary files.
Additionally this changes the behavior of the jj config {edit,set,unset} --user
to target primary config file instead of an interactive window to choose between
that and
conf.d/*.tomlFixes #9541
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.